pathProxy.ts: Implement --proxy-path-passthrough #2563

nhooyr · 2021-01-11T18:28:32Z

nhooyr · 2021-01-11T18:40:20Z

Even with all of this it doesn't work perfectly...

the sockjs-node connection from webpack doesn't use create-react-app's homepage and so is still absolute. I think I'll implement this and also the Referrer solution I referred to in #2222. It's the only way. Very unfortunate.

jsjoeio

Looks good to me! Great work 🎉

Do we want to write tests for this?

doc/FAQ.md

src/node/routes/pathProxy.ts

src/node/cli.ts

jsjoeio · 2021-01-11T19:12:35Z

I tested locally (following the docs) and it works! 🌮

nhooyr · 2021-01-11T19:29:28Z

Unfortunately Referrer isn't set on websocket requests and not always set in general so not something we can rely on. create-react-app will have to fix that to have webpack-dev-server properly work through our proxy. Super unfortunate.

jsjoeio · 2021-01-11T21:19:46Z

Unfortunately Referrer isn't set on websocket requests and not always set in general so not something we can rely on. create-react-app will have to fix that to have webpack-dev-server properly work through our proxy. Super unfortunate.

Do you plan to open up an issue on the create-react-app? (If there isn’t one already)

nhooyr · 2021-01-12T15:33:21Z

@jsjoeio yup see #2565!

also remember don't review until your review is requested again to avoid reviewing WIP changes.
I still have to write the test you requested.

nhooyr · 2021-01-12T15:33:46Z

I also converted back to a draft, didn't know I could do that.

nhooyr · 2021-01-14T16:59:39Z

Tests nearly done! but there's some bizarre fd leak somewhere I need to fix still :(

nhooyr · 2021-01-18T15:42:44Z

This should be good now @jsjoeio!

nhooyr · 2021-01-18T15:45:28Z

omg, i see now, it was leaked-handles. confusing me! updating the commit message.

ci/dev/gen_icons.sh

src/common/util.ts

src/node/app.ts

src/node/routes/index.ts

src/node/routes/pathProxy.ts

test/proxy.test.ts

jsjoeio · 2021-01-19T18:10:54Z

@nhooyr great work and great tests! Left one minor nit/question about an any TS type but non-blocking.

Is this PR intentionally fixing other issues? I see stuff related the the dark mode favicon and the heartbeat error that looks like you fixed. Just wanted to make sure! Otherwise, I can approve

code-asher · 2021-01-19T20:46:26Z

Wooo this is fabulous; the tests are 🔥

For some history on leaked-handles: if I recall correctly I had issues with code-server or one of the child processes not being killed with a SIGTERM or something due to the socket hanging open, I think. So I added this, tracked down the leaked handles, and it resolved the issue.

Or actually it might have been keeping the tests themselves from finishing? I don't quite recall. Something was hanging somewhere.

With all that said I feel a little more confident if it stayed in but I don't feel super strongly either way.

nhooyr · 2021-01-20T07:03:45Z

Is this PR intentionally fixing other issues? I see stuff related the the dark mode favicon and the heartbeat error that looks like you fixed. Just wanted to make sure! Otherwise, I can approve

Yea I screwed up this branch somehow. The favicon commits are unrelated but see my comment re the heartbeat changes.

nhooyr · 2021-01-20T07:04:11Z

With all that said I feel a little more confident if it stayed in but I don't feel super strongly either way.

Happy to leave it in if we can modify the message to indicate it's from that package. Otherwise, it's just confusing if the leak is elsewhere.

Closes #2222

And the concerns surrounding it. Closes #2485

express requires all 4 arguments to be declared for a error handler. It's very unfortunate that our types do not handle this.

The goal is to remove supertest as it does not support typescript well and there's really no good reason for the dependency. Also no websocket testing support.

@code-asher

cc @code-asher

@code-asher

This had me very confused for quite a while until I did a binary search inspection on route/index.ts. Only with the heart.beat line commented out did my tests pass without leaking. They weren't leaking fds but just this heartbeat timer and node of course prints just fds that are active when it detects some sort of leak I guess and that made the whole thing very confusing. These fds are not leaked and will close when node's event loop detects there are no more callbacks to run. no of handles 3 tcp stream { fd: 20, readable: false, writable: true, address: {}, serverAddr: null } tcp stream { fd: 22, readable: false, writable: true, address: {}, serverAddr: null } tcp stream { fd: 23, readable: true, writable: false, address: {}, serverAddr: null } It kept printing the above text again and again for 60s and then the test binary times out I think. I'm not sure if it was node printing the stuff above or if it was a mocha thing. But it was really confusing... cc @code-asher for thoughts on what was going on. edit: It was the leaked-handles import in socket.test.ts!!! Not sure if we should keep it, this was really confusing and misleading.

nhooyr · 2021-01-20T07:07:32Z

Fixed the branch so the favicon changes aren't part of the diff anymore! Was based on a very old version of master sorry!

nhooyr · 2021-01-20T07:21:22Z

Happy to leave it in if we can modify the message to indicate it's from that package. Otherwise, it's just confusing if the leak is elsewhere.

Looks like we can't :(

And https://github.com/myndzi/wtfnode is the preferred utility anyway and has much better output. Opening a PR to switch.

nhooyr · 2021-01-20T07:23:53Z

Also unfortunate and confusing thing about leaked-handles is that it's usage is global as it begins on import instead of letting you call a method to enable. I guess you could require as needed but dynamic requires are a little more annoying. wtfnode just has a global function you call to begin the dumping.

nhooyr · 2021-01-20T07:26:16Z

Oo and we can configure the output further if ever needed! https://github.com/myndzi/wtfnode#configuring-logging

See my comments at coder#2563 (comment)

See my comments at #2563 (comment)

nhooyr · 2021-01-20T07:44:12Z

Awesome, thanks for the reviews guys!

See my comments at #2563 (comment)

nhooyr requested a review from jsjoeio January 11, 2021 18:28

jsjoeio reviewed Jan 11, 2021

View reviewed changes

doc/FAQ.md Show resolved Hide resolved

src/node/routes/pathProxy.ts Show resolved Hide resolved

src/node/cli.ts Show resolved Hide resolved

nhooyr force-pushed the proxy-path-passthrough-0bb9 branch 2 times, most recently from 920d914 to 4ff5443 Compare January 11, 2021 19:37

jsjoeio approved these changes Jan 11, 2021

View reviewed changes

nhooyr marked this pull request as draft January 12, 2021 15:33

nhooyr force-pushed the proxy-path-passthrough-0bb9 branch from e026aee to bf97d37 Compare January 14, 2021 23:13

nhooyr marked this pull request as ready for review January 18, 2021 15:46

nhooyr force-pushed the proxy-path-passthrough-0bb9 branch from b591fa0 to 5723c7a Compare January 18, 2021 15:46

nhooyr requested review from jsjoeio and code-asher January 18, 2021 15:46

nhooyr added this to the v3.8.1 milestone Jan 18, 2021

jsjoeio reviewed Jan 19, 2021

View reviewed changes

nhooyr added 4 commits January 20, 2021 02:06

pathProxy.ts: Implement --proxy-path-passthrough

f169e3a

Closes #2222

FAQ.md: Document --proxy-path-passthrough

497b01b

And the concerns surrounding it. Closes #2485

routes/index.ts: Correctly register wsErrorHandler

ba4a248

express requires all 4 arguments to be declared for a error handler. It's very unfortunate that our types do not handle this.

FAQ.md: Document wds problem with create-react-app and pathProxy.ts

9efcf7f

nhooyr added 8 commits January 20, 2021 02:06

test: Add testutil.HttpServer

ea1949e

The goal is to remove supertest as it does not support typescript well and there's really no good reason for the dependency. Also no websocket testing support.

plugin.test.ts: Switch to testutil.HttpServer

8acb2ae

app.ts: Fix createApp to log all http server errors

d307427

cc @code-asher

test: Rename testutil.ts to httpserver.ts

64e915d

test: Implement integration.ts for near full stack integration testing

240c8e2

test/proxy.test.ts: Implement

60233d0

Formatting and linting fixes

5c06646

nhooyr force-pushed the proxy-path-passthrough-0bb9 branch from 5723c7a to c32d8b1 Compare January 20, 2021 07:06

nhooyr added a commit to nhooyr/code-server that referenced this pull request Jan 20, 2021

test: Switch from leaked-handles to wtfnode

1aaaaa6

See my comments at coder#2563 (comment)

nhooyr added a commit that referenced this pull request Jan 20, 2021

test: Switch from leaked-handles to wtfnode

a397984

See my comments at #2563 (comment)

nhooyr mentioned this pull request Jan 20, 2021

test: Switch from leaked-handles to wtfnode #2604

Merged

nhooyr added a commit that referenced this pull request Jan 20, 2021

test: Switch from leaked-handles to wtfnode

4959eeb

See my comments at #2563 (comment)

nhooyr merged commit 28e98c0 into master Jan 20, 2021

nhooyr deleted the proxy-path-passthrough-0bb9 branch January 20, 2021 07:44

nhooyr added a commit that referenced this pull request Feb 1, 2021

test: Switch from leaked-handles to wtfnode

69cd64c

See my comments at #2563 (comment)

nhooyr added a commit that referenced this pull request Feb 1, 2021

test: Switch from leaked-handles to wtfnode

403cfcc

See my comments at #2563 (comment)

nhooyr added a commit that referenced this pull request Feb 1, 2021

test: Switch from leaked-handles to wtfnode (#2604)

d7f0697

See my comments at #2563 (comment)

nekomeowww mentioned this pull request Aug 24, 2021

Document proxy and shortcomings #4026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pathProxy.ts: Implement --proxy-path-passthrough #2563

pathProxy.ts: Implement --proxy-path-passthrough #2563

nhooyr commented Jan 11, 2021

nhooyr commented Jan 11, 2021

jsjoeio left a comment

jsjoeio commented Jan 11, 2021 •

edited

Loading

nhooyr commented Jan 11, 2021

jsjoeio commented Jan 11, 2021

nhooyr commented Jan 12, 2021

nhooyr commented Jan 12, 2021

nhooyr commented Jan 14, 2021

nhooyr commented Jan 18, 2021

nhooyr commented Jan 18, 2021

jsjoeio commented Jan 19, 2021

code-asher commented Jan 19, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

pathProxy.ts: Implement --proxy-path-passthrough #2563

pathProxy.ts: Implement --proxy-path-passthrough #2563

Conversation

nhooyr commented Jan 11, 2021

nhooyr commented Jan 11, 2021

jsjoeio left a comment

Choose a reason for hiding this comment

jsjoeio commented Jan 11, 2021 • edited Loading

nhooyr commented Jan 11, 2021

jsjoeio commented Jan 11, 2021

nhooyr commented Jan 12, 2021

nhooyr commented Jan 12, 2021

nhooyr commented Jan 14, 2021

nhooyr commented Jan 18, 2021

nhooyr commented Jan 18, 2021

jsjoeio commented Jan 19, 2021

code-asher commented Jan 19, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

nhooyr commented Jan 20, 2021

jsjoeio commented Jan 11, 2021 •

edited

Loading